Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Autocomplete focus scroll #3067

Merged
merged 1 commit into from
May 1, 2024

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Apr 29, 2024

What does this PR do?

  • Fixes issue where navigating dropdown by keyboard does not keep element in view
  • Likely caused by custom implementation of the MUI Autocomplete component
  • I was unable to work out exactly what was causing this, so I'm slightly bypassing this my listening for the focus visible class to be applied, and using this as a proxy for when we should scroll the element into view.

Demo

Screen.Recording.2024-04-29.at.11.47.59.mov

@DafyddLlyr DafyddLlyr changed the base branch from main to dp/fual-autocomplete April 29, 2024 09:18
Copy link

github-actions bot commented Apr 29, 2024

Removed vultr server and associated DNS entries

@@ -281,6 +337,7 @@ export const SelectMultiple = (props: SelectMultipleProps) => {
aria-live="polite"
disableClearable
disableCloseOnSelect
disableListWrap
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also flagged by @ianjon3s as a usability issue - you can now no longer wrap around from final → first option when navigating by keyboard.

@DafyddLlyr DafyddLlyr requested a review from a team April 29, 2024 10:49
Comment on lines +183 to +185
// Using a custom listbox has a breaking effect on the native scroll used by MUI's Autocomplete
// Using a MutationObserver we can listen for MUI applying the focusVisible class
// We then use this to indicate that we need to scroll the focused element into view
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a pretty complex solution to a small UI problem. Tried a few alternatives first, but if there's anything obvious I'm missing here please let me know!

@DafyddLlyr DafyddLlyr marked this pull request as ready for review April 29, 2024 10:52
Copy link
Contributor

@ianjon3s ianjon3s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving from a usability POV, nice work!

@DafyddLlyr DafyddLlyr force-pushed the dp/autocomplete-scroll-fix branch from 8ca955e to 478b9f2 Compare May 1, 2024 09:20
@DafyddLlyr
Copy link
Contributor Author

Merged to #2993 as this will have another pair of eyes on it before going to main

@DafyddLlyr DafyddLlyr merged commit 1c100d3 into dp/fual-autocomplete May 1, 2024
10 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/autocomplete-scroll-fix branch May 1, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants